Clear pending (un)installs when installers are deleted #23427

iansltx · 2024-10-31T01:21:12Z

For unreleased bug #23350, introduced because we're no longer cascade-deleting all related installs (related to #22996, #21654, #22087)

Checklist for submitter

Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)
Added/updated tests
Manual QA for all new/changed functionality

…ending (un)installs on installer delete TODO: * Apply to bulk installer changes * Update tests to ensure that pending (un)installs get deleted

Still TODO: bulk actions

codecov · 2024-10-31T03:54:52Z

Codecov Report

Attention: Patch coverage is 66.66667% with 36 lines in your changes missing coverage. Please review.

Project coverage is 63.15%. Comparing base (471d38b) to head (b289060).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
server/datastore/mysql/software_installers.go	66.03%	24 Missing and 12 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #23427      +/-   ##
==========================================
+ Coverage   63.13%   63.15%   +0.02%     
==========================================
  Files        1555     1556       +1     
  Lines      147093   147290     +197     
  Branches     3725     3669      -56     
==========================================
+ Hits        92861    93019     +158     
- Misses      46886    46910      +24     
- Partials     7346     7361      +15

Flag	Coverage Δ
backend	`64.00% <66.66%> (+0.02%)`	⬆️
frontend	`52.43% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

iansltx · 2024-10-31T17:01:19Z

@noahtalerman for copy, how does this look? Slight tweak from design review to maintain existing "deleting the installer won't uninstall from all hosts" verbiage:

jacobshandling · 2024-10-31T19:25:30Z

...tend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tsx

@@ -18,13 +18,15 @@ const DELETE_SW_INSTALLED_DURING_SETUP_ERROR_MSG =
 interface IDeleteSoftwareModalProps {
  softwareId: number;
  teamId: number;
+  software: any; // TODO


Can we set this to ISoftwarePackage?

Or just pass in the name as a string?

Would have to tweak upstream because according to the type checker we might (but in reality won't) have a VPP app instead, which would result in an undefined installer property, so I can't tighten the type up without other changes (type checker shows ISoftwarePackage | undefined).

The Edit Software modal has the same issue, so probably makes sense to hit both with the same fix.

The way usually handle that is by setting the type as a union with undefined, then use optional chaining to handle the undefined case (which we know won't happen) when referencing it. You could also nullish coallesce to a default case for comfort, though since we know it will always be defined it should be okay either way, of course. So something like

software: ISoftwarePackage | undefined;
...

software?.name ?? "this software"

I'll sometimes leave a comment to the effect of "This will actually always be defined" just to be clear, too.

We have quite a few instances of this situation, which is of course not ideal, but very often cleaning up the typing upstream would involve a) untangling a huge mess of spaghetti and/or b) refactoring our API service layer with generics (something we've discussed) to have actual type checking there. Both would be great to do, but of course, require time, effort, and prioritization.

@jacobshandling Revised to pass the (nullable) name of the package in b289060

iansltx · 2024-10-31T19:26:19Z

Tests pass on this, including new tests, but I still need to manually QA. Added @lucasmrod for BE review, @RachelElysia for FE review, so if my self-QA passes (will write up test plan on the parent ticket) I can get this merged and cherry-picked later today.

jacobshandling · 2024-10-31T19:26:24Z

...tend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tsx

@@ -18,13 +18,15 @@ const DELETE_SW_INSTALLED_DURING_SETUP_ERROR_MSG =
 interface IDeleteSoftwareModalProps {
  softwareId: number;
  teamId: number;
+  software: any; // TODO


Or just pass in the name as a string?

lucasmrod · 2024-10-31T21:10:38Z

server/datastore/mysql/activities.go

 				hsi.status = :software_status_install_pending`,
 		`SELECT
 			COUNT(*) c
 			FROM host_software_installs hsi
-			WHERE hsi.host_id = :host_id AND
+			WHERE hsi.host_id = :host_id AND hsi.software_installer_id IS NOT NULL AND


This is the actual fix for the bug reported by QA wolf right? Should the other changes be linked to different issue?

Underlying cause for the QAWolf bug is that we now need to manually clean up pending installs when installers get deleted, rather than the pending installs getting deleted automatically due to the ON DELETE CASCADE that was previously on host_software_installs.software_installer_id. This fix is belt-and-suspenders so the apparent problem goes away in QAWolf's environment without either a migration or a manual DB query, but without the larger cleanup we'd have invisible pending (un)installs when an installer is deleted, and the install part is a regression from v4.58.0.

Basically, #23350 exists because the implementation of #21654 was incomplete, so this is unreleased whichever way we slice this.

lucasmrod

LGTM!

For unreleased bug #23350, introduced because we're no longer cascade-deleting _all_ related installs (related to #22996, #21654, #22087) # Checklist for submitter - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Added/updated tests - [x] Manual QA for all new/changed functionality

…23445) Cherry-pick of #23427, for unreleased bug #23350 (related to #22996, #21654, #22087)

Hide no-installer pending (un)installs in upcoming activity, delete p…

5ac8a0e

…ending (un)installs on installer delete TODO: * Apply to bulk installer changes * Update tests to ensure that pending (un)installs get deleted

iansltx temporarily deployed to Docker Hub October 31, 2024 01:21 — with GitHub Actions Inactive

Update test to take deletion of pending installs into account

a08dc32

Still TODO: bulk actions

iansltx temporarily deployed to Docker Hub October 31, 2024 03:36 — with GitHub Actions Inactive

iansltx added 2 commits October 31, 2024 09:10

Tweak activities test to cover installer deletion

20c93e9

Add side effects to batch installer deletes

7e605d2

iansltx temporarily deployed to Docker Hub October 31, 2024 15:53 — with GitHub Actions Inactive

Fix lint issues

95617dc

iansltx temporarily deployed to Docker Hub October 31, 2024 16:47 — with GitHub Actions Inactive

Revise copy on delete modal to mention side effects

641c854

iansltx temporarily deployed to Docker Hub October 31, 2024 17:03 — with GitHub Actions Inactive

Fix missing _id in batch set queries

ec9879e

iansltx temporarily deployed to Docker Hub October 31, 2024 17:26 — with GitHub Actions Inactive

Test installer batch deletion side effects

b57ac71

iansltx temporarily deployed to Docker Hub October 31, 2024 19:21 — with GitHub Actions Inactive

iansltx marked this pull request as ready for review October 31, 2024 19:22

iansltx requested review from a team as code owners October 31, 2024 19:22

iansltx assigned lucasmrod and RachelElysia Oct 31, 2024

jacobshandling reviewed Oct 31, 2024

View reviewed changes

jacobshandling previously approved these changes Oct 31, 2024

View reviewed changes

lucasmrod reviewed Oct 31, 2024

View reviewed changes

Pass package name rather than entire package, revise type

b289060

iansltx dismissed jacobshandling’s stale review via b289060 October 31, 2024 21:30

iansltx temporarily deployed to Docker Hub October 31, 2024 21:30 — with GitHub Actions Inactive

lucasmrod approved these changes Oct 31, 2024

View reviewed changes

jacobshandling approved these changes Oct 31, 2024

View reviewed changes

iansltx merged commit 8258d48 into main Oct 31, 2024
26 checks passed

iansltx deleted the 22350-upcoming-activities-fix branch October 31, 2024 23:04

iansltx mentioned this pull request Oct 31, 2024

Cherry-pick: Clear pending (un)installs when installers are deleted #23445

Merged

iansltx added a commit that referenced this pull request Nov 1, 2024

Cherry-pick: Clear pending (un)installs when installers are deleted (#…

59cd7f4

…23445) Cherry-pick of #23427, for unreleased bug #23350 (related to #22996, #21654, #22087)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear pending (un)installs when installers are deleted #23427

Clear pending (un)installs when installers are deleted #23427

iansltx commented Oct 31, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading

iansltx commented Oct 31, 2024

jacobshandling Oct 31, 2024

jacobshandling Oct 31, 2024

iansltx Oct 31, 2024

jacobshandling Oct 31, 2024

jacobshandling Oct 31, 2024

iansltx Oct 31, 2024

iansltx commented Oct 31, 2024

jacobshandling Oct 31, 2024

lucasmrod Oct 31, 2024

iansltx Oct 31, 2024

iansltx Oct 31, 2024

lucasmrod left a comment

Clear pending (un)installs when installers are deleted #23427

Clear pending (un)installs when installers are deleted #23427

Conversation

iansltx commented Oct 31, 2024 • edited Loading

Checklist for submitter

codecov bot commented Oct 31, 2024 • edited Loading

Codecov Report

iansltx commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iansltx commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmrod left a comment

Choose a reason for hiding this comment

iansltx commented Oct 31, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading